perf(table): compile metrics modes once - #1991
Conversation
b46c1c1 to
fead396
Compare
fead396 to
c280bfa
Compare
zeroshade
left a comment
zeroshade
left a comment
There was a problem hiding this comment.
Optimization is behaviorally equivalent (verified by a 3000-schema differential probe against a verbatim reconstruction of the old path), but it strands the PreOrderSchemaVisitor methods as dead production code and silently detaches TestStatsTypes from computeStatsPlan.
Re-review verification: 0 of 1 prior findings confirmed fixed at c280bfa (each verified by mutating the fix and observing the suite go red, not by taking the claim on trust).
Verification performed
Baseline (worktree, PR head c280bfa): `go build ./...` clean; `go vet ./table/ ./table/internal/` clean; `go test ./table/` ok 6.222s; `go test -race ./table/ -run 'TestStatsTypes|TestComputeStatsPlan|TestFileMetrics|TestRollingDataWriter'` ok 3.012s; `go test ./table/internal/` ok 0.475s; `go test -run '^$' -bench '^BenchmarkComputeStatsPlan$' -benchmem ./table/` reproduces the PR's numbers (10000/default: 833963 ns/op, 1312449 B/op, 35 allocs/op; one_percent_overrides: 39 allocs/op). Throwaway probe table/pr1991_probe_test.go (deleted): TestProbeDifferentialStatsPlan compared a verbatim reconstruction of the pre-PR collector+PreOrderVisit path against the new computeStatsPlan over 3000 seeded random schemas -> PASS, no plan or error divergence; TestProbeInvalidUsedOverride and TestProbeInvalidDefault -> PASS with identical error strings. Mutations (each reverted via git checkout): (A) collectStatsPlanField no-op -> TestStatsTypes PASSES, 26 other tests fail; (B) isNested forced false -> full ./table FAILs at TestFileMetrics/TestMetricsModeNonDefaultTrunc; (C) eager return on unused invalid override -> TestComputeStatsPlanIgnoresInvalidUnusedColumnMode FAILs; (D) drop map pre-sizing -> ./table still ok (perf hint only, as expected); (E) drop struct recursion -> TestComputeStatsPlanTraversesNestedFields FAILs.
This review was drafted by an AI-assisted tool and confirmed by an Apache Iceberg Go maintainer. The findings below are observations, not blockers; an Apache Iceberg Go maintainer — a real person — will take the next look at the PR. If you think a finding is mis-applied, please reply on the PR and a maintainer will weigh in.
More on how Apache Iceberg Go handles maintainer review: CONTRIBUTING.md.
| @@ -1706,24 +1708,24 @@ func (a *arrowStatsCollector) Map(m iceberg.MapType, keyResult, valResult func() | |||
There was a problem hiding this comment.
major — Visitor traversal is now dead production code; TestStatsTypes no longer guards computeStatsPlan
computeStatsPlan no longer calls iceberg.PreOrderVisit; it uses the new collectStatsPlanField traversal. That leaves arrowStatsCollector.Schema/Struct/Field/List/Map (lines 1675-1707) and the slice-wrapping Primitive (1754) / Variant (1781) reachable only from arrow_utils_internal_test.go:431. The package now maintains two independent traversals of the same schema, and the single test that asserts exact per-field MetricsMode values validates the orphaned one. Fix: delete the visitor methods and rewrite TestStatsTypes against computeStatsPlan, or keep PreOrderVisit as the only traversal.
Evidence
Mutating collectStatsPlanField to `if true { return }` (production plan always empty): `go test ./table/ -run 'TestStatsTypes$' -v` => `--- PASS: TestStatsTypes (0.00s)` / `ok github.com/apache/iceberg-go/table 0.565s`, while the full package reports 26 `--- FAIL` tests. grep confirms the only arrowStatsCollector+PreOrderVisit construction outside arrow_utils.go is arrow_utils_internal_test.go:432.
| func (a *arrowStatsCollector) Variant(_ iceberg.VariantType) []tblutils.StatisticsCollector { | ||
| func (a *arrowStatsCollector) Primitive(dt iceberg.PrimitiveType) []tblutils.StatisticsCollector { | ||
| colName, ok := a.schema.FindColumnName(a.fieldID) | ||
| if !ok { |
There was a problem hiding this comment.
minor — Primitive resolves the column name twice per field
Primitive calls a.schema.FindColumnName(a.fieldID) at line 1755 solely to compute strings.Contains(colName, "."), then primitiveCollector repeats the identical lookup at line 1726 and repeats the !ok check. In a PR whose stated goal is removing redundant per-field work this is a doubled map lookup on the exact path being optimized. It is currently harmless only because this method is test-only (see the major finding).
| if err != nil { | ||
| if columnModeErrors == nil { | ||
| columnModeErrors = make(map[string]error, len(props)) | ||
| } |
There was a problem hiding this comment.
nit — Override maps pre-sized to len(props) rather than the override count
columnModes and columnModeErrors are allocated with capacity len(props), i.e. the count of ALL table properties, not the count of write.metadata.metrics.column.* keys. Real tables carry many unrelated properties, so this over-allocates the very maps the PR is pre-sizing to save allocations.
| name string | ||
| defaultMode string | ||
| overrideFields int | ||
| }{ |
There was a problem hiding this comment.
nit — Benchmark field overrideFields is a stride, not a field count
The field is named as a count but is used as the loop increment (i += benchmarkCase.overrideFields), so the value 100 yields fieldCount/100 overrides. The arithmetic happens to produce the intended 1% for all three sizes, but the name inverts the meaning and will mislead the next person who tunes it. Rename to overrideStride, or express it as a count and derive the stride.
…s-modes-once Signed-off-by: Minh Vu <vuhoangminh97@gmail.com>
Signed-off-by: Minh Vu <vuhoangminh97@gmail.com>
2 participants
What changed
Why
The old code reparsed the default metrics mode for every primitive and variant field. It also built temporary values through the generic schema visitor. That becomes expensive for wide schemas.
The Java implementation parses these settings once in
MetricsConfig. This change follows the same approach in Go and keeps the compiled modes with the stats-plan build.Benchmark
Measured locally on an Apple M1 Pro with:
go test -run '^$' -bench '^BenchmarkComputeStatsPlan$' -benchmem -count=3 ./tableFor a 10,000-field schema using the default mode:
With 1% column overrides, pre-sizing reduces allocations from 46 to 39 allocs/op.
Checks
go test ./tablego test -race ./tablego test -run '^$' ./...go vet ./tablegolangci-lint run --timeout=10m